An argic horizon is never a Regosol — Luvisol missing-Al-sat default (v0.9.112)#64
Merged
Merged
Conversation
…t default (v0.9.112) Accuracy front B2 (engine). The honest B1 benchmark exposed a correctness bug in the WRB key: a profile with a CONFIRMED argic (clay-illuvial B) horizon could drop to the Regosol catch-all -- the gate for soils with NO diagnostic subsurface horizon -- purely because the eutric/alic split (BS / Al-saturation) was unmeasured, leaving luvisol() at NA so the key skipped it. The fix (surgical, in luvisol(), R/diagnostics-rsg-argic-derived.R): a graceful Al-saturation default mirroring the existing Acrisol BS-fallback. When argic() passes, the clay is high-activity (CEC/clay >= 24), and Al-saturation is UNMEASURED on a B master horizon, default to Luvisol (the generic high-activity argic RSG; Alisol is the high-Al special case that requires positive Al-sat >= 50). Fires only on is.na() -- a measured Luvisol (Al-sat<50) or Alisol (Al-sat>=50) is never overridden. A B-horizon guard keeps it off a Fluvisol's stratified C-layer clay jump (sedimentary, not pedogenic). al_sat_pct stays in $missing so missing_data flags the assumption and Alisol surfaces as an ambiguity. Impact (measured): FEBR-WRB +9 Luvisols (Regosol->Luvisol), 0 regressions; order accuracy 17.8%->21.9% (suite report 18.1->22.6% at n=199), with balanced accuracy, macro-F1 and kappa all up. All 44 canonical fixtures classify byte-identically (the fallback only fires on missing data, which they never have). Scope note from B1: the dominant FEBR-WRB ceiling is missing data (most argic-RSG pedons lack measured clay), not the discriminator the audit imagined, so this is a targeted correctness fix. Verification: full suite 5110/0; R CMD check --as-cran 0/0/0; 22 synthetic B2 tests; git diff over R/ touches ONLY diagnostics-rsg-argic-derived.R (fixtures, key.yaml, other gates untouched). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a WRB 2022 key correctness gap where profiles with a confirmed argic horizon could fall through to the Regosol catch-all when Al-saturation is unmeasured, by adding a targeted fallback in luvisol() plus regression tests and benchmark/report updates.
Changes:
- Add an Al-saturation “missing default” path in
luvisol()for high-activity argic horizons, guarded to B horizons. - Add new B2 regression tests to ensure argic profiles don’t classify as Regosols when Al-saturation is missing.
- Bump version + update release notes and add an updated benchmark suite report.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
R/diagnostics-rsg-argic-derived.R |
Adds the Luvisol fallback logic when al_sat_low is NA, with a B-horizon guard. |
tests/testthat/test-b2-argic-never-regosol.R |
New tests covering the fallback and non-regression cases (measured Alisol/Luvisol, Fluvisol guard). |
NEWS.md |
Documents the 0.9.112 release and the behavioral impact. |
inst/benchmarks/reports/benchmark_suite_v09112.md |
Adds the generated benchmark report for v0.9.112. |
DESCRIPTION |
Bumps package version to 0.9.112. |
ARCHITECTURE.md |
Adds an entry describing the v0.9.112 change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,88 @@ | |||
| # Tests for v0.9.111 "an argic horizon is never a Regosol": the Luvisol | |||
| expect_true(any(grepl("al_sat", lv$missing))) # al_sat still flagged | ||
| expect_true(!is.null(lv$evidence$al_sat_low$details$al_sat_low_default)) | ||
| res <- classify_wrb2022(p, on_missing = "silent") | ||
| expect_equal(res$rsg_or_order, "Luvisols") # was "Regosols" pre-v0.9.111 |
| max_pct = max_al_sat, | ||
| candidate_layers = layers) | ||
|
|
||
| # v0.9.111: graceful Al-saturation fallback. A confirmed argic horizon with |
Comment on lines
+208
to
+212
| if (isTRUE(tests$cec_high$passed) && is.na(tests$al_sat_low$passed)) { | ||
| promoted <- intersect(arg$layers, tests$cec_high$layers) | ||
| desig <- as.character(pedon$horizons$designation)[promoted] | ||
| promoted <- promoted[grepl("B", desig)] | ||
| if (length(promoted) > 0L) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Accuracy front B2 (engine). The honest B1 benchmark (#62) exposed a correctness bug in the WRB key, not the "discriminator" the original audit imagined: a profile with a confirmed argic (clay-illuvial B) horizon could drop to the Regosol catch-all — the gate for soils with no diagnostic subsurface horizon — purely because the eutric/alic split (base saturation / Al-saturation) was unmeasured, leaving
luvisol()atNAso the deterministic key skipped it.The fix (surgical — one gate)
luvisol()(R/diagnostics-rsg-argic-derived.R) gains a graceful Al-saturation default, mirroring the existing Acrisol BS-fallback (v0.9.17). Whenargic()passes, the clay is high-activity (CEC/clay ≥ 24), and Al-saturation is unmeasured on a B master horizon, the profile defaults to Luvisol (the generic high-activity argic RSG; Alisol is the high-Al special case requiring positive Al-sat ≥ 50 evidence). Guards:is.na()— a measured Luvisol (Al-sat<50) or Alisol (Al-sat≥50) is never overridden.argic's clay-increase test false-positives there).al_sat_pctstays in$missing→ surfaces inmissing_data, and Alisol becomes an ambiguity, so the assumption is transparent (the evidence grade is provenance-only, unchanged).Impact (measured, FEBR WRB benchmark)
argic()can't even run; no key change addresses that. This is therefore a targeted correctness fix, not the broad discriminator overhaul the audit imagined.Verification
R CMD check --as-cran0 / 0 / 0.missingflag; measured Alisol/Luvisol unaffected; Alisol abstains (NA) so ordering cedes to the promoted Luvisol; the C-layer (Fluvisol) case is excluded; fixture WRB-landing pins.git diffoverR/touches onlydiagnostics-rsg-argic-derived.R—inst/rules/, the other RSG gates, and all fixtures are untouched.Note: versioned 0.9.112 to avoid a collision with the in-flight AfSP PR #63 (0.9.111).
🤖 Generated with Claude Code